Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate core implementation and testing #1

Merged
merged 12 commits into from
Aug 25, 2023
Merged

Migrate core implementation and testing #1

merged 12 commits into from
Aug 25, 2023

Conversation

AkifumiSato
Copy link
Collaborator

@AkifumiSato AkifumiSato commented Aug 23, 2023

@koichik
Copy link
Member

koichik commented Aug 23, 2023

パッケージの構成ですが、こんな風にしたいと思ってます

  • @location-state/core
  • @location-state/next
  • @locastion-state/react-hook-form

なのでNext.js依存のモジュールはpackages/coreではなくpackages/nextに置きたい感じです

@koichik
Copy link
Member

koichik commented Aug 23, 2023

そうするとpackages/corepackages/nextもファイルはそんなに多くないので、まずはsrc以下にフラットに置いちゃってもいい気がします
types.tsも1ファイルにまとめていいと思うし
それでごちゃついて見えるならsrc/storesとか作っても構いませんが

@koichik
Copy link
Member

koichik commented Aug 23, 2023

この程度の規模のライブラリなら"@/~"ではなく普通の相対パスで十分かなと

@koichik
Copy link
Member

koichik commented Aug 23, 2023

"npm-run-all"要るかな? pnpmかTurborepoか分からないけどそれ相当の機能を持ってる気がするけど
この辺見ると要らなさそう
そして最初からTurborepoでプロジェクト作った方がよさそう?
https://zenn.dev/hayato94087/articles/d2956e662202a7

@koichik
Copy link
Member

koichik commented Aug 23, 2023

packages/core/src/Provider.tsxのファイル名は小文字始まりのprovider.tsxにしたいです
キャメル/パスカルケースではなくケバブケースということで

@koichik
Copy link
Member

koichik commented Aug 23, 2023

packages/core/src/test-utilspackages/test-utilsとかに分離してもいいかもですね

@AkifumiSato
Copy link
Collaborator Author

いくつか修正しました。
turborepoの件は明日ちょっと↓試してみてから考えてみます。
https://turbo.build/repo/docs/getting-started/add-to-project
test-util切り離しもやろうかと思ったけど、turborepo対応してimport { createNavigationMock } from "test-utils";とできるはずな気がするので、合わせて明日やってみます

@koichik
Copy link
Member

koichik commented Aug 23, 2023

どうせならこれで始めたほうがいいのでは?
https://turbo.build/repo/docs/getting-started/create-new

リポジトリは作り直しても構わないので

@AkifumiSato
Copy link
Collaborator Author

試しに作り直してみようとやってみてたんですが、ディレクトリ構成をこのPRの形に戻したりbuild周り整備する方がちょっと面倒そうな気がしてきました。
一旦ここに入れてみるのもやってみます

@AkifumiSato
Copy link
Collaborator Author

AkifumiSato commented Aug 24, 2023

turborepo入れてみました。
キャッシュ効いてるとかなり早くて、testとか3s -> 200msくらいになりました
test-utils分離したんですが、他も内部的な依存に分離すればもっと早くなるはずなのでstoreやsyncerを分離したらさらに早く...と思いつつ、そんなことする段階じゃない気がするんでやってみたい衝動は抑えますww

@koichik
Copy link
Member

koichik commented Aug 24, 2023

乙です!
test-utilsは配布する必要がないので分離する価値がありますが、storeとかはpublishするので分離すべきではないのでは?

@AkifumiSato
Copy link
Collaborator Author

storeを内部パッケージに分離してcoreでimport + bundleするので配布できる+storeに変更がない時はキャッシュが効くようになると思ってます。
(こんだけ実装が小さいとキャッシュ効いても本当に早くなるかわかんない気もしますが...)

packages/test-utils/package.json Show resolved Hide resolved
turbo.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
packages/core/jest.config.mjs Outdated Show resolved Hide resolved
packages/core/src/index.ts Outdated Show resolved Hide resolved
packages/core/src/types.ts Outdated Show resolved Hide resolved
@koichik
Copy link
Member

koichik commented Aug 24, 2023

んー、共有するわけでもなく一つの配布単位を細切れにするメリットを感じないですね
しかもこの程度のサイズ。。。

むしろディレクトリを減らしたいくらい
packages/core/src/store/の下にstorageurlがあるけどそれぞれ2ファイルしかないのでディレクトリ不要じゃないかと
core/src/syncers/もいるのかどうか? unsafe-navigation.tsは実際はSyncerではないし
packages/next/srcの下はディレクトリないわけだし

@AkifumiSato
Copy link
Collaborator Author

はい、僕も分けたいわけじゃないので大丈夫です!
ただturborepoのサンプルや記事読んでると結構細かいprivateパッケージ(雛形だとtsconfigやeslilnt configなど)切ってキャッシュ効かせていく思想を感じたので、かなり小さい構成で並べるのがturborepo wayなのかなぁって思って速度的損益分岐がどの辺か試してみたくなっての感想でした。
にしてもstoreとかは小さいしtsconfigとかまで切らずとも、という気持ちの方が強いですが...

@koichik
Copy link
Member

koichik commented Aug 24, 2023

設定系はキャッシュというより共有という観点で分けたい (一カ所にまとめたい) ってのはありますね

@AkifumiSato
Copy link
Collaborator Author

直下に親を置いて各packageでextendsして、って思ってましたがtsconfig/eslint-config/jestもprivate packageで作りますか?

@AkifumiSato AkifumiSato changed the title coreの実装とテストを移行 Migrate core implementation and testing Aug 24, 2023
@koichik
Copy link
Member

koichik commented Aug 24, 2023

次のステップでやってみてください!
その前に.gitignorenode_modulesがないですね
recoil-sync-nextはこんなだった
https://github.com/recruit-tech/recoil-sync-next/blob/main/.gitignore

@AkifumiSato
Copy link
Collaborator Author

元から僕の環境ではignoreされてたから気づきませんでした。
↑参考に修正しました

Copy link
Member

@koichik koichik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ひとまずここまででマージしてもいいと思います!

@AkifumiSato AkifumiSato merged commit 1a9e236 into main Aug 25, 2023
@AkifumiSato AkifumiSato deleted the setup branch August 26, 2023 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants